-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Move templates out of options #194
WIP: Move templates out of options #194
Conversation
By analyzing the blame information on this pull request, we identified @retlehs, @iamntz and @santouras to be potential reviewers |
</div><!-- #respond --> | ||
</div> | ||
<?php | ||
return apply_filters( 'discourse_replies_html', ob_get_clean() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's better to have ob_get_clean
called outside the apply_filters
(here and everywhere else).
My reasoning is this: if you want only to append some extra markup, you'll have to remember to clean the buffer on return. I think it's an unnecessary extra step...
👍 instead of linking to the wiki on your fork, can you please go ahead and update the links to be based off this repo? and since we require php 5.3, can you add a namespace to the top of that |
Sure. Can I create the wiki on here? |
I've added a namespace and rebased this with the current master branch. I still need to update the links. I can change the namespace if there's something you would rather use than |
namespace looks good, and yeah you can go ahead and add to the wiki on here |
tested this out and it looks good! thanks for doing this. will merge once you've rebased and tag a new version soon |
still need the wiki URLs updated to this repo's 👀 mention me/reply once that's done and i'll merge |
I had a brief discussion about this with @scossar in a separate channel, so I'll repost here:
What's your take on this @retlehs ? This does make theme tweaking more of a developer-y action. |
customizing HTML templates is developer-y to begin with, and having the markup editable in the admin is awkward. it also doesn't make sense for that HTML to be saved to the options table in the database. |
@retlehs I've updated the URLs |
Based on this commit to the 10up fork of wp-discourse: https://github.com/10up/wp-discourse/commit/5c9d43c4333e136204d5a3b07192f4b368c3f518
This PR moves the html templates out of the plugin's options. If this is implemented, html templates will no longer be customizable through the options page. They can be customized through the theme using the filters that are applied to each function. A link to the documentation of how to do that is added to the settings page. Here is a tentative version: https://github.com/scossar/wp-discourse/wiki/Template-Customization
This PR also changes the plugin's text-domain from
discourse
towp-discourse
.